Skip to content

Add SubprocessEvaluator for process-isolated evaluation - #77

Open
odelliab wants to merge 4 commits into
skydiscover-ai:mainfrom
odelliab:feature/subprocess-evaluator
Open

Add SubprocessEvaluator for process-isolated evaluation#77
odelliab wants to merge 4 commits into
skydiscover-ai:mainfrom
odelliab:feature/subprocess-evaluator

Conversation

@odelliab

@odelliab odelliab commented Jun 7, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds SubprocessEvaluator — a new evaluator that runs each candidate in a separate
Python subprocess, providing process-level isolation without requiring Docker.

Problem

When evaluating candidate programs that can corrupt process state (e.g. CUDA kernels with illegal
memory access, C extensions that segfault, memory corruption), the in-process
Evaluator allows one bad candidate to poison all subsequent evaluations. For GPU
workloads, cudaErrorIllegalAddress is sticky — once triggered, the CUDA context is
permanently corrupted and all further operations fail.

Solution

SubprocessEvaluator provides a middle ground between Evaluator (fast, no isolation) and
ContainerizedEvaluator (full Docker):

Evaluator Isolation Overhead Setup
Evaluator None ~0ms None
SubprocessEvaluator Process ~100-200ms None
ContainerizedEvaluator Container High Dockerfile required
  • Each evaluate() call spawns a fresh Python subprocess
  • Child process gets its own CUDA context / address space
  • If a candidate crashes, only the subprocess dies
  • Same evaluate(program_path) -> dict interface as Evaluator

Usage

set evaluator.subprocess_isolation: true in config YAML.

The auto-detection in create_evaluator() checks this flag after Harbor/Container detection but before falling back to in-process.

##Test
Includes 6 tests covering: successful evaluation, noisy stdout parsing, crash isolation, exception handling, crash-then-success recovery, and timeout behavior.

Usage

evaluator:
  subprocess_isolation: true
  evaluation_file: my_evaluator.py
  timeout: 300

When evaluating candidate programs that can corrupt process state (e.g.
CUDA kernels with illegal memory access, C extensions that segfault),
the in-process Evaluator allows one bad candidate to poison all
subsequent evaluations within the same CUDA context.

SubprocessEvaluator provides a middle ground between the in-process
Evaluator (no isolation) and ContainerizedEvaluator (requires Docker):

- Each evaluate() call spawns a fresh Python subprocess
- Child process gets its own CUDA context / address space
- If a candidate crashes, only the subprocess dies
- ~100-200ms overhead per evaluation for process startup
- Same evaluate(program_path) -> dict interface as Evaluator

Usage: set `evaluator.subprocess_isolation: true` in config YAML.

The auto-detection in create_evaluator() checks this flag after
Harbor/Container detection but before falling back to in-process.

Includes 6 tests covering: successful evaluation, noisy stdout parsing,
crash isolation, exception handling, crash-then-success recovery, and
timeout behavior.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces SubprocessEvaluator to run candidate evaluations in isolated Python subprocesses, preventing crashes from affecting the parent process. Feedback on the implementation highlights several key areas for improvement: handling non-JSON-serializable return types (such as EvaluationResult or numpy arrays) in the wrapper script, fixing a potential resource leak and NameError during temporary file creation, replacing the blocking run_in_executor pattern with a non-blocking asyncio.create_subprocess_exec to avoid orphaned processes on timeout, and removing unnecessary sys.path modifications in the parent process.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread skydiscover/evaluation/subprocess_evaluator.py
Comment thread skydiscover/evaluation/subprocess_evaluator.py
Comment thread skydiscover/evaluation/subprocess_evaluator.py
Comment thread skydiscover/evaluation/subprocess_evaluator.py Outdated
odelliab and others added 3 commits June 7, 2026 12:30
- Wrapper template: handle EvaluationResult objects via to_dict(),
  use default=str for non-serializable types (numpy arrays etc.)
- Fix temp file leak: assign temp_path before write, guard cleanup
  with `if temp_path and os.path.exists(temp_path)`
- Replace run_in_executor + subprocess.run with asyncio.create_subprocess_exec
  for proper timeout handling (proc.kill() + await proc.wait() on timeout)
- Remove unnecessary sys.path modification in parent process
  (child gets eval_dir via PYTHONPATH env var)
- Remove unused subprocess import
Reuse the existing SafeJSONEncoder from checkpoint_manager (with an
inline fallback if the import fails in the subprocess) instead of the
generic default=str approach.
- Fix black violations in subprocess_evaluator.py (argument-per-line,
  slice spacing)
- Replace sys.modules stubbing in test_subprocess_evaluator.py with
  normal imports — the stub for skydiscover.config poisoned the module
  cache and caused ImportError in tests collected afterwards

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@shubham3-ucb

Copy link
Copy Markdown
Collaborator

Thanks for this - a subprocess isolation tier between the in-process and containerized evaluators is a real gap, and the wiring through create_evaluator is clean. Before merge, a few items where the subprocess path silently diverges from the in-process Evaluator it is documented to mirror. I reproduced these against the branch.

1. Timeout leaks orphaned processes (high). The child is created without start_new_session=True, and the timeout path calls proc.kill(), which SIGKILLs only the python -c wrapper. Any process the candidate's evaluate() forks (torch DataLoader workers, multiprocessing pools, NCCL) survives and reparents to PID 1. Repro on this branch: a timed-out eval returned {'error': 0.0, 'timeout': True} while its grandchild stayed alive with PPID 1, still holding its resources. This is the exact leak the isolation feature exists to prevent. Fix: pass start_new_session=True and on timeout os.killpg(os.getpgid(proc.pid), signal.SIGKILL) (guard ProcessLookupError) then await proc.wait().

2. cascade_evaluation is ignored (high). EvaluatorConfig.cascade_evaluation defaults to True, and the in-process Evaluator runs evaluate_stage1 -> threshold -> evaluate_stage2 -> merge. The wrapper template only calls mod.evaluate(...), so combined_score silently differs under isolation for the bundled cascade benchmarks (circle_packing, signal_processing, txn_scheduling, gpu_mode/shared_eval). Repro: same program scored 0.9 in-process vs 0.1 under subprocess. Please replicate the stage logic in the child (or share one implementation), or fail loudly on cascade configs rather than downgrading silently.

3. EvaluationResult.artifacts are dropped on the round-trip (high). to_dict nests artifacts under metrics['artifacts'], but from_dict assigns the whole dict to metrics and leaves artifacts={}. Downstream feedback loops (adaevolve/controller.py, gepa_native/controller.py reading artifacts.get('feedback')) go silently empty under isolation. Fix: make from_dict pop 'artifacts' back out so to_dict/from_dict are symmetric.

4. Forced cwd (medium). The subprocess passes cwd=dirname(evaluation_file); the in-process Evaluator keeps the run cwd. Evals that read datasets by cwd-relative path silently fail after toggling the flag. Inherit the parent cwd, or make both paths identical and document it.

5. Image mode broken (medium). is_image_mode is stored but the <temp>.image_path sidecar that the in-process Evaluator writes (and that benchmarks/image_gen/sky_festival reads) is never created, so every image-mode candidate scores 0.0 under isolation. Write and unlink the sidecar mirroring evaluator.py.

6. Fragile JSON extraction (medium). stdout.rfind('\\n{') only tolerates preceding stdout noise that ends in a newline; a non-newline-terminated write immediately before the result (progress bars, print(end='')) turns a valid score into {'error': 0.0}. Prefer a sentinel-delimited result line from the wrapper, or scan for the last balanced JSON object.

Tests. The timeout assertion timeout is True or error == 0.0 is tautological since every failure path sets error=0.0 - it passes even if the timeout mechanism breaks; use assert result.metrics.get('timeout') is True. Also evaluate_batch and concurrent isolation have no coverage. The highest-leverage addition would be a parity test that runs one benchmark through both Evaluator and SubprocessEvaluator and diffs the result - that single test covers items 2, 3, 4, and 5.

Nice work overall - none of these are structural, the design is sound, and it is complementary to the existing evaluators rather than redundant. Formatting (black/isort) and the current suite (164 tests) are green. Happy to re-review once the parity gaps are closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants